Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace old Mesh page with New Mesh page #7321

Merged
merged 47 commits into from
May 17, 2024
Merged

Conversation

jshaughn
Copy link
Collaborator

@jshaughn jshaughn commented Apr 29, 2024

This PR replaces the old Mesh page with the new Mesh page, removing the undocumented feature flag. It should contain a releasable version.

Closes #7298
Closes #6431

TESTING
The PR removes the new mesh page feature flag, and so it should appear as the default Mesh Page. The goal of the first release of the page is not perfection, but it should be solid, usable, and useful. It replaces the old mesh page, the non-Kiali parts of the About page, and demotes the Overview Control plane card to a standard card (although it will still be labeled as a control plane). It should be able to handle standard deployments, multi-cluster deployments, canary upgrades, and externally deployed infra (you don't necessarily have to test all of these things).

@jshaughn jshaughn self-assigned this Apr 29, 2024
@jshaughn jshaughn changed the title Kiali7298 Replace old Mesh page with New Mesh page Apr 29, 2024
@jshaughn jshaughn force-pushed the kiali7298 branch 5 times, most recently from 3385e1b to 96ba4c8 Compare May 6, 2024 03:01
@jshaughn jshaughn force-pushed the kiali7298 branch 3 times, most recently from cb0d842 to 76a5f6c Compare May 6, 2024 21:09
@ferhoyos
Copy link
Contributor

ferhoyos commented May 7, 2024

It is not related with this PR, but I think istio status should take into account both istiod and istio pod status. Currently it only takes into account istiod, show it shows a healthy status if there is something wrong with istio pod:

image

image

@jshaughn I can create a different issue or fix it in this PR, as you consider.

@jshaughn
Copy link
Collaborator Author

jshaughn commented May 7, 2024

It is not related with this PR, but I think istio status should take into account both istiod and istio pod status. Currently it only takes into account istiod, show it shows a healthy status if there is something wrong with istio pod:

@ferhoyos +1, a different issue or a PR against this PR is fine. This PR is mostly stable with changes only resulting from adding test code or making fixes. I made your suggested change about treating Data Plane and Kiali as Healthy (basically, if there is no health assigned then it defaults to Healthy). I also applied your suggestion about embedding the JSON config into the Data Plane expandable sections.

Copy link
Contributor

@ferhoyos ferhoyos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just pair of comments, you are on the good path to replace old mesh page @jshaughn! I will do a PR to your PR with my suggestion about istio health consideration.

frontend/src/pages/Mesh/target/TargetPanelNode.tsx Outdated Show resolved Hide resolved
frontend/src/pages/Mesh/MeshPage.tsx Outdated Show resolved Hide resolved
@jshaughn jshaughn force-pushed the kiali7298 branch 2 times, most recently from 687c406 to b948d27 Compare May 9, 2024 18:18
@jshaughn jshaughn added test: front-end/cypress PR adds/updates front-end tests (unit and/or cypress automation ) test: back-end/integration PR adds/updates back-end tests (unit and/or integration) labels May 13, 2024
@jshaughn jshaughn marked this pull request as ready for review May 13, 2024 20:32
@jshaughn jshaughn requested review from ferhoyos and removed request for ferhoyos May 14, 2024 23:51
@hhovsepy hhovsepy self-requested a review May 15, 2024 05:58
@hhovsepy
Copy link
Contributor

Tested on Multicluster, single cluster and in Ambient mode, works fine, currently checking on OCP cluster (will take longer due to env issues):
Screenshot from 2024-05-15 09-40-18
Screenshot from 2024-05-15 09-22-03
Screenshot from 2024-05-15 09-15-16
Screenshot from 2024-05-15 09-14-17

@hhovsepy
Copy link
Contributor

Tested on OCP, working fine, just one question, the component's versions are undefined, does any config missing:
Screenshot from 2024-05-15 12-53-53

at least Grafana has a version:
Screenshot from 2024-05-15 12-55-23

@leandroberetta
Copy link
Contributor

Did a first run on Minikube and works well and I really like how it looks and how the information is grouped.

image

The collapsible namespaces for the data plane is awesome.

One minor thing that I guess can be improve is that when you have a namespace with no inbount and outbound traffic, it can be a small indication of this situation (to avoid the size of the empty graphs):

image

Related to versions, I do notice that Jaeger and Grafana versions are unknown.

I will start the proper code review and probably a canary/mc testing too now, but I'm very happy with this new mesh page!.

jshaughn and others added 15 commits May 15, 2024 13:43
issue with DataPlane nodes for canary versions, and possibly other
situations.
- fix double-revision in istiod name
- add I18N to UNKNOWN constant, and use as opposed to literal
- mesh side panel
  - for clarity, increase indent on infra
  - for conciseness, remove namespace for non-istiod
  - sort infra, and move istiod(s) to top
  - remove the "no canary" test from overview, it's irrelevant
- add a missing @sleep-app to an overview test
- Add version info for grafana via API, if possible
  - note that tracing version may be unavailable via api, or may need a different approach
- reduce whitespace for dataplane namespace, when it lacks chart metrics
@jshaughn
Copy link
Collaborator Author

@hhovsepy , @leandroberetta , @ferhoyos , I pushed changes that should now show Grafana version, if available (tracing version is still not solved). Also, I reduced the vertical whitespace in an expanded data plane namespace, when it lacks traffic for the charts.

@jshaughn
Copy link
Collaborator Author

Hi @hhovsepy , @leandroberetta , We've incorporated your feedback. If public URLs can be determined then we can show the Grafana and Jaeger version info:
image

And we've fixed the excessive whitespace when no metrics are available for a data plane namespace:
image

The CI failures are limited to the know issue with multi-primary. If you are OK with the state of things, please approve and we'll get this in for some soak time prior to sprint end. If you have more suggestions we can follow up with other issues for the Epic. Of course if you see a blocking issue we'll fix it in this PR. Thanks!

@hhovsepy
Copy link
Contributor

Hi @jshaughn , the components versions are fixed, thanks.
However the cluster version is Unknown, tested on OCP:
Screenshot from 2024-05-16 21-32-43

@jshaughn
Copy link
Collaborator Author

However the cluster version is Unknown, tested on OCP

Hi @hhovsepy Hmm, As shown in my screenshot above, I am seeing the cluster version on my CRC 4.14, using the default "Kubernetes". I'm not sure why you don't get it, but I guess it means that you don't have it set in the return from GetStatus(). Try a call to <kiali>/api/status and I guess you don't see the cluster in the external_services section? We may have to investigate that difference, but we do show it if it's set. I think this should probably be a follow-on issue.

@hhovsepy
Copy link
Contributor

@jshaughn right, probably this is a separate issue as API does not return cluster version.

Copy link
Contributor

@leandroberetta leandroberetta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@jshaughn jshaughn merged commit d458c06 into kiali:master May 17, 2024
8 of 9 checks passed
@jshaughn jshaughn deleted the kiali7298 branch May 17, 2024 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test: back-end/integration PR adds/updates back-end tests (unit and/or integration) test: front-end/cypress PR adds/updates front-end tests (unit and/or cypress automation )
Projects
Development

Successfully merging this pull request may close these issues.

Remove legacy Mesh page Show mesh configuration per controlplane
4 participants